🐛 set RecheckTime when minSuccessTime is defined #163

haoqing0110 · 2025-11-05T08:03:03Z

Summary

Related issue(s)

open-cluster-management-io/ocm#1237

Fixes #

Summary by CodeRabbit

Refactor
- Unified and simplified rollout recheck timing and semantics for clearer timeout/soak handling and more accurate next-check scheduling.
Tests
- Updated and added tests to validate the revised recheck timing behavior across multiple rollout strategies and edge cases.

openshift-ci · 2025-11-05T08:03:08Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: haoqing0110
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

coderabbitai · 2025-11-05T08:03:23Z

Walkthrough

Replaced TimeOutTime with RecheckTime in ClusterRolloutStatus, added calculateRecheckTime to compute next check times, and refactored recheck calculation so minRecheckAfter derives its result from per-cluster RecheckTime values. Tests updated accordingly.

Changes

Cohort / File(s)	Summary
Core Rollout Logic `pkg/apis/cluster/v1alpha1/rollout.go`	Replaced public field `TimeOutTime metav1.Time` with `RecheckTime metav1.Time` in `ClusterRolloutStatus`. Added `calculateRecheckTime(startTime metav1.Time, duration time.Duration) metav1.Time`. Refactored `determineRolloutStatus()` and related flows to set and use `RecheckTime` (soak end or timeout) and replaced uses of `getTimeOutTime`. Updated `minRecheckAfter(rolloutClusters []ClusterRolloutStatus) *time.Duration` to compute minimum from `RecheckTime` fields. Fixed comment typo.
Tests `pkg/apis/cluster/v1alpha1/rollout_test.go`	Updated expected structs and test logic to use `RecheckTime` instead of `TimeOutTime` across rollout scenarios. Added `TestMinRecheckAfter()` to validate selection of the minimum `RecheckTime` (nil/empty/mixed cases). Adjusted time-based expectations to align with new recheck semantics.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Inspect determineRolloutStatus() and callers to ensure RecheckTime is set for the correct statuses (succeeded soak end vs. timeout/soak for others).
Verify calculateRecheckTime nil-handling matches previous semantics where no timeout/soak is intended.
Confirm minRecheckAfter correctly computes the minimum positive duration from RecheckTime and that all callers were updated appropriately.
Review updated tests, especially TestMinRecheckAfter(), for comprehensive coverage of edge cases (nil, empty, mixed).

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is incomplete; the Summary section is empty, and the Fixes field lacks a specific issue number despite referencing an issue URL.	Complete the Summary section with a clear description of the changes and populate the 'Fixes #' field with the specific issue number (1237).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the main change: setting RecheckTime when minSuccessTime is defined, which aligns with the refactoring to replace TimeOutTime with RecheckTime.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 81959d2 and aab7c47.

📒 Files selected for processing (2)

pkg/apis/cluster/v1alpha1/rollout.go (8 hunks)
pkg/apis/cluster/v1alpha1/rollout_test.go (26 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/apis/cluster/v1alpha1/rollout_test.go (1)

pkg/apis/cluster/v1alpha1/rollout.go (8)

Failed (33-33)

Progressing (29-29)

ToApply (27-27)

ClusterRolloutStatus (42-57)

TimeOut (36-36)

Succeeded (31-31)

RolloutResult (62-73)

RolloutClock (19-19)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: unit
GitHub Check: integration
GitHub Check: verify

🔇 Additional comments (8)

pkg/apis/cluster/v1alpha1/rollout_test.go (4)

145-151: LGTM: Test expectations correctly updated to use RecheckTime.

The systematic replacement of TimeOutTime with RecheckTime in test expectations is correct. The calculated values are accurate:

cluster4: fakeTime_60s + 90s = fakeTime30s ✓

cluster2 (timeout): fakeTime_120s + 90s = fakeTime_30s ✓

564-630: Excellent test coverage for mixed soak and timeout scenarios.

This new test case comprehensively validates the RecheckTime calculation when clusters have different states:

cluster1: In 60s soak period, 30s remaining → RecheckTime = fakeTime30s

cluster2: Progressing with 90s timeout, 30s remaining → RecheckTime = fakeTime30s

cluster3: Progressing with 90s timeout, 60s remaining → RecheckTime = fakeTime60s

cluster4: Outside soak period, correctly excluded

The expected RecheckAfter of 30s correctly reflects the minimum across clusters 1, 2, and 3.

1639-1697: Good addition: ProgressivePerGroup test with minSuccessTime.

This test case validates the interaction between soak periods and group-based rollout:

cluster1: In soak period with RecheckTime set

cluster2: Outside soak period, correctly excluded from rollout

cluster3: Progressing with timeout tracking

The test confirms that RecheckAfter correctly derives from the minimum RecheckTime (30s).

2474-2548: Comprehensive test coverage for minRecheckAfter function.

The TestMinRecheckAfter test thoroughly validates all scenarios:

Single cluster with RecheckTime

Multiple clusters with different RecheckTimes (correctly selects minimum)

Mixed soak and timeout periods

Nil/empty inputs (correctly returns nil)

Clusters without RecheckTime (correctly ignored)

This provides strong confidence in the refactored minRecheckAfter implementation.

pkg/apis/cluster/v1alpha1/rollout.go (4)

53-56: Improved field naming and documentation.

Renaming TimeOutTime to RecheckTime better reflects the dual purpose:

For succeeded status: tracks when the minSuccessTime (soak time) period ends

For progressing/failed status: tracks when the timeout occurs

The updated documentation clearly explains both semantics.

475-479: Correct soak period handling for succeeded clusters.

The logic properly calculates and sets RecheckTime for succeeded clusters still within their soak period, ensuring they remain in rolloutClusters until the minSuccessTime elapses.

500-515: Well-designed calculateRecheckTime helper function.

This consolidates the recheck time calculation logic with good semantics:

Handles nil startTime by using current time (reasonable default for new clusters)

Returns nil for maxTimeDuration (indicating no timeout/soak period)

Simple, focused responsibility

606-623: Simplified and more intuitive minRecheckAfter implementation.

The refactored function is cleaner and more consistent:

Derives minimum from per-cluster RecheckTime values (single source of truth)

Correctly filters out past recheck times with recheckDuration > 0 check

Returns nil when no future rechecks are needed

This eliminates the need to pass timeout and minSuccessTime parameters, as those are already encoded in each cluster's RecheckTime.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Signed-off-by: Qing Hao <[email protected]>

haoqing0110 · 2025-11-06T08:10:09Z

/assign @qiujian16
cc @youngbupark

openshift-ci bot requested a review from deads2k November 5, 2025 08:03

openshift-ci bot requested a review from qiujian16 November 5, 2025 08:03

set RecheckTime when minSuccessTime is defined

aab7c47

Signed-off-by: Qing Hao <[email protected]>

haoqing0110 force-pushed the refactor-rollout-rechecktime branch from 81959d2 to aab7c47 Compare November 5, 2025 08:19

haoqing0110 mentioned this pull request Nov 6, 2025

enhance the MWRS rollout gaps open-cluster-management-io/ocm#1237

Open

openshift-ci bot assigned qiujian16 Nov 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

🐛 set RecheckTime when minSuccessTime is defined #163

🐛 set RecheckTime when minSuccessTime is defined #163

Uh oh!

haoqing0110 commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

openshift-ci bot commented Nov 5, 2025

Uh oh!

coderabbitai bot commented Nov 5, 2025 •

edited

Loading

Uh oh!

haoqing0110 commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

🐛 set RecheckTime when minSuccessTime is defined #163

Are you sure you want to change the base?

🐛 set RecheckTime when minSuccessTime is defined #163

Uh oh!

Conversation

haoqing0110 commented Nov 5, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related issue(s)

Summary by CodeRabbit

Uh oh!

openshift-ci bot commented Nov 5, 2025

Uh oh!

coderabbitai bot commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

haoqing0110 commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

haoqing0110 commented Nov 5, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Nov 5, 2025 •

edited

Loading